Skip to content

cpplint: Use pypi version - #1871

Merged
rok-cesnovar merged 2 commits into
stan-dev:developfrom
HaoZeke:pypi_cpplint
May 8, 2020
Merged

cpplint: Use pypi version#1871
rok-cesnovar merged 2 commits into
stan-dev:developfrom
HaoZeke:pypi_cpplint

Conversation

@HaoZeke

@HaoZeke HaoZeke commented May 7, 2020 •
edited by rok-cesnovar
Loading

Copy link
Copy Markdown
Contributor

Summary

Python 2 has been depreciated for quite a while now, and it isn't mandated by any other part of the process.

The fork on pypi is probably the one most people will expect anyway, so have simply replaced the existing cpplint with that, adding to the ignore rules to ensure no further changes are required at this point.

P.S.: I would actually prefer to follow the flag guidelines, but let me know if that would be too invasive of a change.

Tests

With the new cpplint:

Category 'build/header_guard' errors found: 2805
Category 'build/include_order' errors found: 3301
Category 'build/include_what_you_use' errors found: 23
Category 'build/namespaces' errors found: 3
Category 'runtime/string' errors found: 36
Total errors found: 6168
make: *** [make/cpplint:5: cpplint] Error 1

With the old one:

Total errors found: 0

The most common errors are essentially related to ordering of system libraries and the fact that the newer guidelines like the guards to be in a different format, as well as the suggestion of having a C style string for global and static strings:

#ifndef STAN_MATH_REV_FUN_VARIANCE_HPP -> #ifndef STAN_MATH_REV_FUN_VARIANCE_HPP_
#endif -> #endif // STAN_MATH_REV_FUN_VARIANCE_HPP_
For a static/global string constant, use a C style string instead: "const char MAJOR_VERSION[]". [4]
stan/math/rev/functor/cvodes_ode_data.hpp(195): error cpplint: [build/include_what_you_use] Add #include <utility> for move [4]

Have currently ignored these suggestions (flag errors format):

  • build/include_order 3301
  • build/header_guard 2805
  • build/namespaces 3
  • runtime/string 36
  • build/include_what_you_use 23

To ensure make cpplint does not error out, but am happy to update as per the linter guidelines as well.

TL;DR- Post PR:

Total errors found: 0

i.e. no change.

Side Effects

Will have to make a similar PR to clean up the make/cpplint file in stan-dev/stan as well.

Release notes

Updated cpplint to conform to pypi

Checklist

  • Math issue #(issue number) N/A

  • Copyright holder: Rohit Goswami <rg0swami[at]yahoo.com>

    The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
    - Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
    - Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

  • the basic tests are passing

    • unit tests pass (to run, use: ./runTests.py test/unit)
    • header checks pass, (make test-headers)
    • dependencies checks pass, (make test-math-dependencies)
    • docs build, (make doxygen)
    • code passes the built in C++ standards checks (make cpplint)

The doxygen test has issues (depreciation warnings), but not related to this PR, and not with the version of doxygen specified.

  • the code is written in idiomatic C++ and changes are documented in the doxygen N/A

  • the new changes are tested

@HaoZeke HaoZeke mentioned this pull request May 7, 2020
Merged
3 tasks
@bob-carpenter

bob-carpenter commented May 7, 2020

Copy link
Copy Markdown
Member

Thanks for the patch. Is this just configuring the Python 3 version to work like our Python 2 version did? Or does this change code to comply? I couldn't quite follow from the PR notes.

There's always a copyright holder for code. That's the author by default, or their employer if they assign copyright to code to their employer (as most employers, including most universities, require).

We asked explicitly to get a record of copyright holders and to make sure contributors were OK licensing it open source. I intentionally wanted to keep the contributors as copyright holders so that nobody could try to seize control of the whole project.

What I think is probably a better approach now is to remind users that they have to have copyright and contribute according to our licensing from the GitHub terms of service, to which we can point in the PR template. That'd stop me from nagging everyone to fill in the PR template and I think we're still OK on copyright. This is what @seantalts did proactively for the stanc3 repo, but we haven't adopted that practice in the main math or stan repos.

@HaoZeke

HaoZeke commented May 7, 2020

Copy link
Copy Markdown
Contributor Author

Sorry for the confusion.

This PR just switches to the python 3 compatible version and adds ignore rules to ensure no changes are required in the code right now.

I have updated the PR with the relevant copyright.

@SteveBronder

SteveBronder commented May 7, 2020

Copy link
Copy Markdown
Collaborator

@serban-nicusor-toptal do we need to update the jenkins machines to use python3 as well?

@serban-nicusor-toptal

serban-nicusor-toptal commented May 7, 2020

Copy link
Copy Markdown
Contributor

All the machines should have python3 installed, does this PR require any change to the Jenkinsfile ?

@rok-cesnovar

rok-cesnovar commented May 7, 2020

Copy link
Copy Markdown
Member

No. jenkins calls make cpplint. This changes what happens underneath.

@serban-nicusor-toptal

serban-nicusor-toptal commented May 7, 2020

Copy link
Copy Markdown
Contributor

Good, I'll double-check all machines now for python3 to be sure. I'll comment here when done.

rok-cesnovar
rok-cesnovar approved these changes May 7, 2020
View reviewed changes

@rok-cesnovar rok-cesnovar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Thanks for this patch. I don't think we need the /samples folder but it's not huge so we can also leave it in. Lets wait for @serban-nicusor-toptal to re-check the status of Python3 on Jenkins and then merge.

@rok-cesnovar

rok-cesnovar commented May 7, 2020

Copy link
Copy Markdown
Member

I would actually prefer to follow the flag guidelines, but let me know if that would be too invasive of a change.

I agree. I feel that this way of doing it by first replacing cpplint and then applying the changes to follow guidelines will be easier to review.

@serban-nicusor-toptal

serban-nicusor-toptal commented May 7, 2020

Copy link
Copy Markdown
Contributor

Hey, here are the results:

AWS Linux - Yes ( python3 resolves to Python 3.5.2 )
AWS Windows - Yes ( python resolves to Python 3.8.2 )
gelman-group-mac - No ( No root access )
gelman-group-win2 - Yes ( python resolves to Python 3.8.2 )
old-imac - Yes ( python3 resolves to Python 3.4.2 )
gelman-group-win-new - Machine down
gelman-group-linux - Machine down

Will this create a problem on mac ? I don't have root access to it.

@rok-cesnovar

rok-cesnovar commented May 8, 2020

Copy link
Copy Markdown
Member

Restarted the tests so that the upstream Stan branch points to the stan-dev/stan PR.

@serban-nicusor-toptal can we remove the mac machine for cpplint until someone either gives you acess or upgrades to python 3.

@serban-nicusor-toptal

serban-nicusor-toptal commented May 8, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Done.
The cpplint stage is set to any agent, I have limited gelman-group-mac to only build jobs that ask for mac tags gpu ocaml osx so cpplint will happen on linux now. ( The linux machines build all the jobs that don't have an agent/label assigned )

@rok-cesnovar

rok-cesnovar commented May 8, 2020

Copy link
Copy Markdown
Member

@serban-nicusor-toptal Is the hold up in this test https://jenkins.mc-stan.org/blue/organizations/jenkins/Stan/detail/PR-2916/3/pipeline/142
because of this change of the mac worker?

@serban-nicusor-toptal

serban-nicusor-toptal commented May 8, 2020

Copy link
Copy Markdown
Contributor

Hey, sadly no. Jenkins will spin up new Linux nodes as the queue is growing, lately, we've had a bit of delay because of almost all machines in AWS being used. ( Note: We are using Spot Instances which are 70-80% cheaper but if someone pays the full price aka on-demand for the instance we lose it )

AWS Logs Example:

Repeated errors have occurred processing the launch specification "m5dn.4xlarge, ami-0a5df599f96078980, Linux/UNIX, us-east-2b while launching spot instance". It will not be retried for at least 13 minutes. Error message: There is no Spot capacity available that matches your request. (Service: AmazonEC2; Status Code: 500; Error Code: InsufficientInstanceCapacity)

How to figure this out without having access to AWS ?
image
In Jenkins you will see that it requested target: 2 but nodes: 0 is still 0 because it doesn't have any machine available.

I will add new types of machines ( a bit more expensive ) to get us more chances for a machine type, tho lately most of the machine types are being depleted.
There's also the option to switch to on-demand instances but the price difference will be quite big.

More information about Spot Requests

@rok-cesnovar

rok-cesnovar commented May 8, 2020

Copy link
Copy Markdown
Member

Oh yeah, that makes sense with a lot of people working from home these days. I guess. Thanks for the explanation.

@stan-buildbot

stan-buildbot commented May 8, 2020

Copy link
Copy Markdown
Contributor
Name Old Result New Result Ratio Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan 5.14 5.14 1.0 0.02% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan 0.02 0.02 1.01 0.66% faster
eight_schools/eight_schools.stan 0.09 0.09 0.99 -0.87% slower
gp_regr/gp_regr.stan 0.23 0.23 1.01 0.55% faster
irt_2pl/irt_2pl.stan 6.44 6.43 1.0 0.18% faster
performance.compilation 85.5 84.15 1.02 1.59% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan 7.56 7.64 0.99 -1.09% slower
pkpd/one_comp_mm_elim_abs.stan 19.87 21.11 0.94 -6.23% slower
sir/sir.stan 92.42 93.96 0.98 -1.67% slower
gp_regr/gen_gp_data.stan 0.05 0.05 0.99 -0.79% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan 3.0 2.98 1.01 0.74% faster
pkpd/sim_one_comp_mm_elim_abs.stan 0.31 0.31 1.01 0.67% faster
arK/arK.stan 1.75 1.75 1.0 -0.35% slower
arma/arma.stan 0.77 0.73 1.05 4.54% faster
garch/garch.stan 0.57 0.57 1.01 0.92% faster
Mean result: 0.999705511184

Jenkins Console Log
Blue Ocean
Commit hash: 5aaef8bb9bb35011ff934fed9bc9bdb7858e3592

Machine information ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

@rok-cesnovar
rok-cesnovar merged commit 1f82629 into stan-dev:develop May 8, 2020
@rok-cesnovar

rok-cesnovar commented May 8, 2020

Copy link
Copy Markdown
Member

@HaoZeke congrats on your first merged PR to the Stan Math repository. 🎉

@HaoZeke

HaoZeke commented May 8, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thanks @rok-cesnovar and everyone else. It was fun! Should I start applying more of the default linter rules to the code? Perhaps that should be an issue or a discourse post, since people who are working with the code right now should be informed or they will suddenly see a lot of broken tests.

Update: Made a discourse post

@rok-cesnovar rok-cesnovar mentioned this pull request May 9, 2020
Closed
@HaoZeke HaoZeke mentioned this pull request May 13, 2020
Closed
@rok-cesnovar rok-cesnovar mentioned this pull request Jul 20, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rok-cesnovar rok-cesnovar rok-cesnovar approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@HaoZeke @bob-carpenter @SteveBronder @serban-nicusor-toptal @rok-cesnovar @stan-buildbot